Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a parameter cacheValuesOnly to Cache.make.* #160

Closed
wants to merge 2 commits into from

Conversation

kajebiii
Copy link

This PR is a different approach for #159

Differences between previous #159

There is another problem: if you call refresh while refreshValue is ongoing, it will do nothing. (#159 (comment))

  • Cache has only one refresh function. Therefore, we don't need refreshValue like the previous PR.

I think one of the issues here is we are treating refresh inconsistently with get. (#159 (comment))

  • If cacheValuesOnly is true, cache can store values only. It means if get returns an error, then the promise (made in refresh) failed or lookup returned an error when the cache is empty. Therefore, we can delete the inconsistency from previous PR.

@adamgfraser
Copy link
Contributor

This is another way to do it. The irregularity with both of these approaches to some extent is that it isn't true that we are caching values only. If computing the value takes five seconds everyone who gets the value within those five seconds is going to receive the same "cached" value computed whether it is a success or failure.

@kajebiii
Copy link
Author

Thank you for your reply.

If computing the value takes five seconds everyone who gets the value within those five seconds is going to receive the same "cached" value computed whether it is a success or failure.

Can you explain it in more detail? I don't know what your point is.

In this PR, if we call get then (when cacheValuesOnly = true)

  • Empty cache
    • Lookup Success -> replace with lookup exit and return success value (Same "cached" value)
    • Lookup Error -> keep cache state empty and return error value ("uncached" value but it looks not weird since cacheValuesOnly = true so that the error can not be in cache)
  • Existing cache value
    • return cache value (no lookup call since there is a cache value)

@adamgfraser
Copy link
Contributor

Consider that fiber A calls get for a value at t=0 for a value that takes 5 seconds to compute. At t=1 fiber B calls get for the same value. At t=5` the computation of the value fails with an error. At this point both fiber A and fiber B receive the single error value from the single computation. From the perspective of an external observer the value has been "cached" even though supposedly it would not be cached for error values.

And similarly a fiber that calls get at t=4.9 will receive the value from the single computation whereas a fiber that calls get at t=5.1 will not receive the value from the single computation.

Basically the nature of the asynchronous cache forces us to decide at the time we begin computing the value whether we will cache it or not, at least for other fibers that attempt to compute the value at the same time, but we don't know if the result is the failure until the time we finish computing the value.

@kajebiii
Copy link
Author

kajebiii commented Oct 16, 2023

I see your point.

Since we don't know the result is the failure or not until the time we compute the value, this approach is also unlikely to be a solution... 😢 (Or is there an another option to solve the above problem?)

#159 (comment)

Actually thinking more about this, there is another problem: if you call refresh while refreshValue is ongoing, it will do nothing.

Any ideas for a different approach? What really matters for us is only the case where we had a good value that we want to refresh it only with another good one. We don't really care of cases where the value didn't exist or was an error. A simple set would actually be enough as we could lookup ourselves and override it.

  • Like pierre said, what is really important for us is that successful value is not deleted from the cache due to an error when refresh. Any ideas for a different approach?
  • If there is no another approach, we maybe need to implement our own custom cache for this purpose...

@kyri-petrou
Copy link
Contributor

Closing as stale, feel free to reopen if needed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants